fix: add missing fields to native errors in new arch #43649

vonovak · 2024-03-26T08:27:44Z

Summary:

With 0.74.rc-5 one bug which related to errors was fixed (#41950). However, the fix introduced another one: the shape of Error objects that come from native modules has changed. This PR attempts to fix that, though it's not (yet) doing it in a way that would be 100% compatible with the old arch.

The problem was observed on iOS, not sure what the situation is on Android but believe it's okay there.

edit: on Android, there are no issues but the message field is enumerable, so that part is different from ios (see logs below).

Consider this code, where error is produced from a promise rejection inside of a native module.

  console.log(
    'own properties: ',
    JSON.stringify(Object.getOwnPropertyNames(error), null, 2),
  );
  console.log(
    'own enumerable properties: ',
    JSON.stringify(Object.entries(error), null, 2),
  );

These are the results for

Old architecture

 LOG  Running "google-one-tap-example" with {"rootTag":1,"initialProps":{}}
 LOG  own properties:  [
  "stack",
  "code",
  "message",
  "domain",
  "userInfo",
  "nativeStackIOS"
]
 LOG  own enumerable properties:  [
  [
    "code",
    "-5"
  ],
  [
    "message",
    "RNGoogleSignIn: The user canceled the sign in request., Error Domain=com.google.GIDSignIn Code=-5 \"The user canceled the sign-in flow.\" UserInfo={NSLocalizedDescription=The user canceled the sign-in flow.}"
  ],
  [
    "domain",
    "com.google.GIDSignIn"
  ],
  [
    "userInfo",
    {
      "NSLocalizedDescription": "The user canceled the sign-in flow."
    }
  ],
  [
    "nativeStackIOS",
    [
      "0   ReactTestApp                        0x0000000102f4a6d8 RCTJSErrorFromCodeMessageAndNSError + 112",
      "1   ReactTestApp                        0x0000000102eeedd0 __41-[RCTModuleMethod processMethodSignature]_block_invoke_2.73 + 152",
      "2   ReactTestApp                        0x0000000102e2ae24 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548",
      "3   ReactTestApp                        0x0000000102e2aa8c -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184",
      "4   ReactTestApp                        0x0000000102e2a8e0 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236",
      "5   ReactTestApp                        0x0000000102e28628 __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100",
      "6   ReactTestApp                        0x0000000102dc9d80 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132",
...
    ]
  ]
]

RN 74 rc-5 (with bridgeless on)

  (NOBRIDGE) LOG  Bridgeless mode is enabled
 (NOBRIDGE) LOG  Running "google-one-tap-example" with {"rootTag":1,"initialProps":{"concurrentRoot":true},"fabric":true}
 (NOBRIDGE) LOG  own properties:  [
  "stack",
  "message",
  "cause"
]
 (NOBRIDGE) LOG  own enumerable properties:  [
  [
    "cause",
    {
      "code": "-5",
      "message": "RNGoogleSignIn: The user canceled the sign in request., Error Domain=com.google.GIDSignIn Code=-5 \"The user canceled the sign-in flow.\" UserInfo={NSLocalizedDescription=The user canceled the sign-in flow.}",
      "nativeStackIOS": [
        "0   ReactTestApp                        0x00000001023a7b38 RCTJSErrorFromCodeMessageAndNSError + 112",
        "1   ReactTestApp                        0x00000001026cf774 ___ZZN8facebook5react15ObjCTurboModule13createPromiseERNS_3jsi7RuntimeENSt3__112basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEU13block_pointerFvU13block_pointerFvP11objc_objectEU13block_pointerFvP8NSStringSH_P7NSErrorEEENK3$_0clES4_RKNS2_5ValueEPSQ_m_block_invoke.57 + 332",
        "2   ReactTestApp                        0x0000000102270958 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548",
        "3   ReactTestApp                        0x00000001022705c0 -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184",
        "4   ReactTestApp                        0x0000000102270414 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236",
        "5   ReactTestApp                        0x000000010226e15c __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100",
        "6   ReactTestApp                        0x000000010220f328 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132",
...
      ],
      "domain": "com.google.GIDSignIn",
      "userInfo": {
        "NSLocalizedDescription": "The user canceled the sign-in flow."
      }
    }
  ]
]

with the diff from this PR

 (NOBRIDGE) LOG  own properties:  [
  "stack",
  "message",
  "code",
  "nativeStackIOS",
  "domain",
  "userInfo"
]
 (NOBRIDGE) LOG  own enumerable properties:  [
  [
    "code",
    "-5"
  ],
  [
    "nativeStackIOS",
    [
      "0   ReactTestApp                        0x000000010083b8f8 RCTJSErrorFromCodeMessageAndNSError + 112",
      "1   ReactTestApp                        0x0000000100b63534 ___ZZN8facebook5react15ObjCTurboModule13createPromiseERNS_3jsi7RuntimeENSt3__112basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEU13block_pointerFvU13block_pointerFvP11objc_objectEU13block_pointerFvP8NSStringSH_P7NSErrorEEENK3$_0clES4_RKNS2_5ValueEPSQ_m_block_invoke.57 + 332",
      "2   ReactTestApp                        0x0000000100704718 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548",
      "3   ReactTestApp                        0x0000000100704380 -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184",
      "4   ReactTestApp                        0x00000001007041d4 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236",
      "5   ReactTestApp                        0x0000000100701f1c __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100",
      "6   ReactTestApp                        0x00000001006a30e8 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132",
...
    ]
  ],
  [
    "domain",
    "com.google.GIDSignIn"
  ],
  [
    "userInfo",
    {
      "NSLocalizedDescription": "The user canceled the sign-in flow."
    }
  ]
]

You see there is a change compared to old arch because message is no longer own enumerable property. If that needs to change (I guess it should), it'd be nice if someone more familiar with JSI pointed me in the right direction. Even with this inconsistency, the PR is an improvement and would be nice to have this fix included in the next RC.

This is output from Chrome's console for completeness, just to have something to compare to:

let err = new Error('hello')
undefined
Object.getOwnPropertyNames(err)
> ['stack', 'message']
Object.entries(err)
> []

Changelog:

Test Plan:

Tested locally with an example app running RN 74-rc 5

analysis-bot · 2024-03-26T08:59:46Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	18,450,670	+1
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,815,645	-1
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 00725fa
Branch: main

cortinico · 2024-03-27T11:35:06Z

If you don't mind I'd rather wait for @cipolleschi to review and import this one since he has already context on the previous change. He'll be back next week

facebook-github-bot · 2024-04-03T11:12:25Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2024-04-03T11:13:46Z

This change makes sense to me and it basically unwraps the cause object so that errors are the similar in the two architectures.

for:

You see there is a change compared to old arch because message is no longer own enumerable property. If that needs to change (I guess it should), it'd be nice if someone more familiar with JSI pointed me in the right direction.

That goes a little bit out of my experience, so I need to understand better how errors are created... perhaps @RSNara can reply faster than me investigating the source code.

cipolleschi · 2024-04-03T11:20:12Z

.../react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm

@@ -219,7 +219,10 @@ id convertJSIValueToObjCObject(jsi::Runtime &runtime, const jsi::Value &value, s
  NSString *message = jsErrorDetails[@"message"];

  auto jsError = createJSRuntimeError(runtime, [message UTF8String]);
-  jsError.asObject(runtime).setProperty(runtime, "cause", convertObjCObjectToJSIValue(runtime, jsErrorDetails));
+  for (NSString* key in jsErrorDetails) {


out of curiosity, what happens if we add a line like:

jsError.asObject(runtime).setProperty(runtime, [@"message" UTF8String], convertObjCObjectToJSIValue(runtime, message))

🤔 this might duplicate the message field in the own property (so it should be skipped as there is already another message field), but it should also add the message to the enumerable ones...

@cipolleschi this is already happening - the for loop iterates over jsErrorDetails, including the message entry

facebook-github-bot · 2024-04-04T19:15:53Z

@cipolleschi merged this pull request in 98b1331.

github-actions · 2024-04-04T19:15:56Z

This pull request was successfully merged by @vonovak in 98b1331.

^{When will my fix make it into a release? | How to file a pick request?}

Summary: With 0.74.rc-5 one bug which related to errors was fixed (#41950). However, the fix introduced another one: the shape of Error objects that come from native modules has changed. This PR attempts to fix that, though it's not (yet) doing it in a way that would be 100% compatible with the old arch. The problem was observed on iOS, not sure what the situation is on Android but believe it's okay there. edit: on Android, there are no issues but the `message` field is enumerable, so that part is different from ios (see logs below). Consider this code, where `error` is produced from a promise rejection inside of a native module. ```ts console.log( 'own properties: ', JSON.stringify(Object.getOwnPropertyNames(error), null, 2), ); console.log( 'own enumerable properties: ', JSON.stringify(Object.entries(error), null, 2), ); ``` These are the results for <details> <summary>Old architecture</summary> ``` LOG Running "google-one-tap-example" with {"rootTag":1,"initialProps":{}} LOG own properties: [ "stack", "code", "message", "domain", "userInfo", "nativeStackIOS" ] LOG own enumerable properties: [ [ "code", "-5" ], [ "message", "RNGoogleSignIn: The user canceled the sign in request., Error Domain=com.google.GIDSignIn Code=-5 \"The user canceled the sign-in flow.\" UserInfo={NSLocalizedDescription=The user canceled the sign-in flow.}" ], [ "domain", "com.google.GIDSignIn" ], [ "userInfo", { "NSLocalizedDescription": "The user canceled the sign-in flow." } ], [ "nativeStackIOS", [ "0 ReactTestApp 0x0000000102f4a6d8 RCTJSErrorFromCodeMessageAndNSError + 112", "1 ReactTestApp 0x0000000102eeedd0 __41-[RCTModuleMethod processMethodSignature]_block_invoke_2.73 + 152", "2 ReactTestApp 0x0000000102e2ae24 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548", "3 ReactTestApp 0x0000000102e2aa8c -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184", "4 ReactTestApp 0x0000000102e2a8e0 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236", "5 ReactTestApp 0x0000000102e28628 __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100", "6 ReactTestApp 0x0000000102dc9d80 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132", ... ] ] ] ``` </details> <details> <summary>RN 74 rc-5 (with bridgeless on)</summary> ``` (NOBRIDGE) LOG Bridgeless mode is enabled (NOBRIDGE) LOG Running "google-one-tap-example" with {"rootTag":1,"initialProps":{"concurrentRoot":true},"fabric":true} (NOBRIDGE) LOG own properties: [ "stack", "message", "cause" ] (NOBRIDGE) LOG own enumerable properties: [ [ "cause", { "code": "-5", "message": "RNGoogleSignIn: The user canceled the sign in request., Error Domain=com.google.GIDSignIn Code=-5 \"The user canceled the sign-in flow.\" UserInfo={NSLocalizedDescription=The user canceled the sign-in flow.}", "nativeStackIOS": [ "0 ReactTestApp 0x00000001023a7b38 RCTJSErrorFromCodeMessageAndNSError + 112", "1 ReactTestApp 0x00000001026cf774 ___ZZN8facebook5react15ObjCTurboModule13createPromiseERNS_3jsi7RuntimeENSt3__112basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEU13block_pointerFvU13block_pointerFvP11objc_objectEU13block_pointerFvP8NSStringSH_P7NSErrorEEENK3$_0clES4_RKNS2_5ValueEPSQ_m_block_invoke.57 + 332", "2 ReactTestApp 0x0000000102270958 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548", "3 ReactTestApp 0x00000001022705c0 -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184", "4 ReactTestApp 0x0000000102270414 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236", "5 ReactTestApp 0x000000010226e15c __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100", "6 ReactTestApp 0x000000010220f328 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132", ... ], "domain": "com.google.GIDSignIn", "userInfo": { "NSLocalizedDescription": "The user canceled the sign-in flow." } } ] ] ``` </details> <details> <summary>with the diff from this PR</summary> ``` (NOBRIDGE) LOG own properties: [ "stack", "message", "code", "nativeStackIOS", "domain", "userInfo" ] (NOBRIDGE) LOG own enumerable properties: [ [ "code", "-5" ], [ "nativeStackIOS", [ "0 ReactTestApp 0x000000010083b8f8 RCTJSErrorFromCodeMessageAndNSError + 112", "1 ReactTestApp 0x0000000100b63534 ___ZZN8facebook5react15ObjCTurboModule13createPromiseERNS_3jsi7RuntimeENSt3__112basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEU13block_pointerFvU13block_pointerFvP11objc_objectEU13block_pointerFvP8NSStringSH_P7NSErrorEEENK3$_0clES4_RKNS2_5ValueEPSQ_m_block_invoke.57 + 332", "2 ReactTestApp 0x0000000100704718 +[RNGoogleSignin rejectWithSigninError:withRejector:] + 548", "3 ReactTestApp 0x0000000100704380 -[RNGoogleSignin handleCompletion:serverAuthCode:withError:withResolver:withRejector:fromCallsite:] + 184", "4 ReactTestApp 0x00000001007041d4 -[RNGoogleSignin handleCompletion:withError:withResolver:withRejector:fromCallsite:] + 236", "5 ReactTestApp 0x0000000100701f1c __40-[RNGoogleSignin signIn:resolve:reject:]_block_invoke_2 + 100", "6 ReactTestApp 0x00000001006a30e8 __35-[GIDSignIn addCompletionCallback:]_block_invoke_2 + 132", ... ] ], [ "domain", "com.google.GIDSignIn" ], [ "userInfo", { "NSLocalizedDescription": "The user canceled the sign-in flow." } ] ] ``` </details> You see there is a change compared to old arch because `message` is no longer own enumerable property. If that needs to change (I guess it should), it'd be nice if someone more familiar with JSI pointed me in the right direction. Even with this inconsistency, the PR is an improvement and would be nice to have this fix included in the next RC. This is output from Chrome's console for completeness, just to have something to compare to: ``` let err = new Error('hello') undefined Object.getOwnPropertyNames(err) > ['stack', 'message'] Object.entries(err) > [] ``` bypass-github-export-checks ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [IOS] [FIXED] - add missing fields to native errors in new arch For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #43649 Test Plan: Tested locally with an example app running RN 74-rc 5 Reviewed By: cortinico Differential Revision: D55690184 Pulled By: cipolleschi fbshipit-source-id: 60a857b9871af888dcd526782b5e6b73c07c051a

fix: add missing fields to native errors in new arch

0b63b95

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 26, 2024

brentvatne requested review from cortinico and cipolleschi March 26, 2024 18:57

cipolleschi approved these changes Apr 3, 2024

View reviewed changes

cipolleschi reviewed Apr 3, 2024

View reviewed changes

facebook-github-bot closed this in 98b1331 Apr 4, 2024

facebook-github-bot added the Merged This PR has been merged. label Apr 4, 2024

cipolleschi mentioned this pull request Apr 5, 2024

[0.74] fix: add missing fields to native errors in new arch reactwg/react-native-releases#199

Closed

This was referenced Apr 9, 2024

[RN][iOS] Cherry Pick #43757 and #43994 #44007

Merged

[iOS][Local] Fix breaking changes in 0.74-rc.8 #44025

Closed

NOUIY mentioned this pull request May 15, 2024

[Snyk] Upgrade react-native from 0.63.3 to 0.74.0 NOUIY/react-native-macos#1294

Open

This was referenced Jun 28, 2024

[CI] Move iOS Executors to M1 #45213

Closed

[Hermes] Bump Hermes #45220

Merged

cipolleschi mentioned this pull request Jul 22, 2024

[RN][0.74]Fix XROS build #45580

Merged

cipolleschi mentioned this pull request Jul 31, 2024

[RN][Hermes] Fore Hermes versions for slices through env var #45840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing fields to native errors in new arch #43649

fix: add missing fields to native errors in new arch #43649

vonovak commented Mar 26, 2024 •

edited

Loading

analysis-bot commented Mar 26, 2024

cortinico commented Mar 27, 2024

facebook-github-bot commented Apr 3, 2024

cipolleschi commented Apr 3, 2024

cipolleschi Apr 3, 2024

vonovak Apr 3, 2024

facebook-github-bot commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

fix: add missing fields to native errors in new arch #43649

fix: add missing fields to native errors in new arch #43649

Conversation

vonovak commented Mar 26, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

analysis-bot commented Mar 26, 2024

cortinico commented Mar 27, 2024

facebook-github-bot commented Apr 3, 2024

cipolleschi commented Apr 3, 2024

cipolleschi Apr 3, 2024

Choose a reason for hiding this comment

vonovak Apr 3, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Apr 4, 2024

github-actions bot commented Apr 4, 2024

vonovak commented Mar 26, 2024 •

edited

Loading